[WIP] The time bounded snapshot feature #1844
Conversation
- Added the "--snapshot-duration" CLI option. Default value 0 indicates that the snapshot will be limited by the --max-cache-size parameter only. If the value is more than 0, the cyclic buffer for the snapshot will be limited by both the series of messages duration and the maximum cache size parameter. - To override the upper bound by total messages size, the "--maximum-cache-size" CLI option can be settled to 0. Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
MichaelOrlov
changed the title
Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
MichaelOrlov
force-pushed
the
morlov/add_max_snapshot_duration_option
branch
3 times, most recently
from
caa3c08 to
0e076ec
Compare
Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
MichaelOrlov
force-pushed
the
morlov/add_max_snapshot_duration_option
branch
from
0e076ec to
351b529
Compare
|
@reinzor I've made good progress on this PR. I would say I've made all the required changes for this feature. However, I didn't have time to test it and write unit tests. The help here would be really appreciated. Feel free to cherry-pick commits from this PR or create a fork above it. |
There was a problem hiding this comment.
@reinzor FYI, i left a few comments for your reference when you take this over.
| @@ -1,4 +1,4 @@ | |||
| // Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
| // Copyright 2021 Amazonhe t.com, Inc. or its affiliates. All Rights Reserved. | |||
| ROSBAG2_CPP_LOG_ERROR_STREAM("Invalid arguments for the MessageCacheCircularBuffer. " | ||
| "Both max_bytes_size and max_cache_duration are zero."); |
There was a problem hiding this comment.
if this is debug message, it should include some extra debug information that the following exception cannot throw as in the message? otherwise, it is the same information with the following exception, so maybe debugging message here is not really useful since user can see the exact same message via exception?
| use_cache_ = storage_options.max_cache_size > 0u || | ||
| (storage_options.snapshot_mode && storage_options.snapshot_duration.nanoseconds() > 0); |
There was a problem hiding this comment.
checking storage_options.snapshot_mode is redundant?
| use_cache_ = storage_options.max_cache_size > 0u || | |
| (storage_options.snapshot_mode && storage_options.snapshot_duration.nanoseconds() > 0); | |
| use_cache_ = storage_options.max_cache_size > 0u || storage_options.snapshot_duration.nanoseconds() > 0; |
| @@ -49,4 +49,7 @@ player_params_node: | |||
| max_cache_size: 9898 | |||
| storage_preset_profile: "resilient" | |||
| snapshot_mode: false | |||
| snapshot_duration: | |||
| sec: 1 | |||
| nsec: 500000000 | |||
| custom_data: ["key1=value1", "key2=value2"] | |||
There was a problem hiding this comment.
Requires a newline at the end of the file?
This PR adds the "--snapshot-duration" CLI option.
The snapshot duration CLI option is specified as a floating point value in seconds.
The default value 0.0 indicates that the snapshot will be limited by the
--max-cache-sizeparameter only. If the value is more than 0.0, the cyclic buffer for the snapshot will be limited by both the series of messages duration and the maximum cache size parameter. To override the upper bound by total messages size, the--maximum-cache-sizeCLI option can be settled to 0.